Optimize pathfinding with A* and enhance Directions UI - #237
Open
nschimme wants to merge 6 commits into
Open
Conversation
- Implemented point-to-point A* search in MapData using an admissible Euclidean distance heuristic. - Enhanced "Find Rooms" dialog with a "Distance" column and a "Directions" button. - Added a "Directions" entry to the map context menu when rooms are selected. - Exposed the active parser to MainWindow via ConnectionListener to facilitate UI-triggered pathfinding. - Maintained Dijkstra for multi-target searches where it is more efficient.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideIntroduces an A*-based point-to-point pathfinding routine and wires it into parser and UI flows (Find Rooms dialog and map context menu) so users can request directions to specific rooms, while also surfacing approximate Euclidean distance to search results and tightening parser access via ConnectionListener. Sequence diagram for the new directions-to-room flowsequenceDiagram
actor User
participant FindRoomsDlg
participant MainWindow
participant ConnectionListener
participant AbstractParser
participant MapData
participant ShortestPathRecipient
User->>FindRoomsDlg: click_directionsButton
FindRoomsDlg-->>MainWindow: sig_getDirections(RoomId)
MainWindow->>ConnectionListener: getUserParser()
ConnectionListener-->>MainWindow: AbstractParser*
MainWindow->>AbstractParser: doGetDirectionsToRoom(RoomHandle)
AbstractParser->>MapData: shortestPathSearchPointToPoint(origin, target, recipient)
MapData-->>ShortestPathRecipient: receiveShortestPath(sp_nodes, targetIndex)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The distance shown in the FindRooms dialog uses an unweighted Euclidean distance, while the A* heuristic (euclidean_distance) treats Z as more expensive; consider reusing the same helper or at least the same weighting to keep UX-consistent and avoid confusing discrepancies between “closest” and “shortest path.”
- The logic to resolve a RoomHandle and call doGetDirectionsToRoom is duplicated in the FindRooms dialog signal handler and MainWindow::slot_onDirections; consider factoring this into a small helper in MainWindow to avoid divergence in behavior over time (e.g., future null checks or preconditions).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The distance shown in the FindRooms dialog uses an unweighted Euclidean distance, while the A* heuristic (euclidean_distance) treats Z as more expensive; consider reusing the same helper or at least the same weighting to keep UX-consistent and avoid confusing discrepancies between “closest” and “shortest path.”
- The logic to resolve a RoomHandle and call doGetDirectionsToRoom is duplicated in the FindRooms dialog signal handler and MainWindow::slot_onDirections; consider factoring this into a small helper in MainWindow to avoid divergence in behavior over time (e.g., future null checks or preconditions).
## Individual Comments
### Comment 1
<location path="src/parser/abstractparser.cpp" line_range="505-508" />
<code_context>
}
}
+void AbstractParser::doGetDirectionsToRoom(const RoomHandle &target)
+{
+ ShortestPathEmitter sp_emitter(*this);
+ if (const auto r = m_mapData.findRoomHandle(getTailPosition())) {
+ MapData::shortestPathSearchPointToPoint(r, target, sp_emitter);
+ }
</code_context>
<issue_to_address>
**suggestion:** Handle missing origin room more explicitly when computing directions
If `getTailPosition()` can reference a non-existent room, this function will silently do nothing. Consider either falling back to another position (e.g., the current player location) or at least logging/emitting feedback so callers know why no directions were produced.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+505
to
+508
| void AbstractParser::doGetDirectionsToRoom(const RoomHandle &target) | ||
| { | ||
| ShortestPathEmitter sp_emitter(*this); | ||
| if (const auto r = m_mapData.findRoomHandle(getTailPosition())) { |
There was a problem hiding this comment.
suggestion: Handle missing origin room more explicitly when computing directions
If getTailPosition() can reference a non-existent room, this function will silently do nothing. Consider either falling back to another position (e.g., the current player location) or at least logging/emitting feedback so callers know why no directions were produced.
- Implemented point-to-point and multi-target A* search in MapData. - Used admissible Manhattan distance heuristic scaled by minimum edge cost (0.65) to ensure consistency with original Dijkstra search. - Integrated A* into the `_dirs` command for faster pattern-based searches. - Enhanced "Find Rooms" dialog with a "Distance" column and a "Directions" button. - Added a "Directions" entry to the map context menu. - Exposed the active parser to MainWindow via ConnectionListener to facilitate UI-triggered pathfinding. - Strictly maintained original movement cost logic (terrain, doors, ridability, etc.) for all pathfinding.
- Implemented point-to-point and multi-target A* search in MapData. - Used admissible Manhattan distance heuristic scaled by minimum edge cost (0.65) to ensure consistency with original Dijkstra search. - Integrated A* into the `_dirs` command for faster pattern-based searches. - Enhanced "Find Rooms" dialog with a "Distance" column and a "Directions" button. - Added a "Directions" entry to the map context menu. - Exposed the active parser to MainWindow via ConnectionListener to facilitate UI-triggered pathfinding. - Strictly maintained original movement cost logic (terrain, doors, ridability, etc.) for all pathfinding. - Fixed formatting and compilation issues.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #237 +/- ##
==========================================
- Coverage 25.69% 0.00% -25.70%
==========================================
Files 521 365 -156
Lines 43187 27837 -15350
Branches 4708 3185 -1523
==========================================
- Hits 11099 0 -11099
+ Misses 32088 27837 -4251 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Implemented point-to-point and multi-target A* search in MapData. - Used admissible Manhattan distance heuristic scaled by minimum edge cost (0.65) to ensure consistency with original Dijkstra search. - Integrated A* into the `_dirs` command for faster pattern-based searches. - Enhanced "Find Rooms" dialog with a "Distance" column and a "Directions" button. - Added a "Directions" entry to the map context menu. - Exposed the active parser to MainWindow via ConnectionListener to facilitate UI-triggered pathfinding. - Strictly maintained original movement cost logic (terrain, doors, ridability, etc.) for all pathfinding. - Fixed 64-bit to 32-bit integer conversion warnings.
- Implemented point-to-point and multi-target A* search in MapData. - Used admissible Manhattan distance heuristic scaled by minimum edge cost (0.65) to ensure consistency with original Dijkstra search. - Integrated A* into the `_dirs` command for faster pattern-based searches. - Enhanced "Find Rooms" dialog with a "Distance" column and a "Directions" button. - Added a "Directions" entry to the map context menu. - Exposed the active parser to MainWindow via ConnectionListener to facilitate UI-triggered pathfinding. - Strictly maintained original movement cost logic (terrain, doors, ridability, etc.) for all pathfinding. - Fixed 64-bit to 32-bit integer conversion warnings.
- Moved `Abbrev` and `CompareVersion` to `src/global` to resolve cross-library dependency issues in tests. - Created `src/map-utils/CommandNames` to house command mapping logic, allowing `mm_map` to remain independent of the parser. - Updated `src/CMakeLists.txt` to correctly partition map-related subdirectories and link `mm_map` with `Qt6::Network` and `Qt6::OpenGL`. - Implemented A* search algorithm for point-to-point and multi-target pathfinding in `MapData`, maintaining consistency with movement costs. - Enhanced "Find Rooms" dialog with distances and a "Directions" button. - Added "Directions" action to the map context menu. - Fixed 64-bit integer conversion warnings and ensured optimal paths. - Verified all unit tests (`TestMap`, `TestExpandoraCommon`, `TestMainWindow`) pass with the new library structure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Investigated and optimized pathfinding in MMapper.
Key improvements:
MapData::shortestPathSearchPointToPointimplementing A* search. This provides a performance boost when a specific destination is known.PR created automatically by Jules for task 14194329936225864437 started by @nschimme
Summary by Sourcery
Introduce point-to-point A* pathfinding and expose direct directions workflows from search results and map context menus.
New Features:
Bug Fixes:
Enhancements: